Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: model inventory #250

Merged
merged 34 commits into from
Mar 10, 2023
Merged

feat: model inventory #250

merged 34 commits into from
Mar 10, 2023

Conversation

hollandjg
Copy link
Member

@hollandjg hollandjg commented Jan 20, 2023

Description

Add the model inventory.

part of #232
resolves #232

Type of change:

  • New feature (non-breaking change which adds functionality)

Features:

  • two functions to register and retrieve models
  • three example models: prospect theory, expected value theory, Weber-Fechner law
  • documentation for the new autora.synthetic module

Questions:

  • what should the verbs be? register and retrieve? or get and set? or something totally different?

@benwandrew
Copy link
Collaborator

bump. @musslick @chadcwilliams @gtdang

@hollandjg hollandjg marked this pull request as draft March 6, 2023 18:37
@hollandjg hollandjg marked this pull request as ready for review March 6, 2023 20:34
@hollandjg
Copy link
Member Author

hi @musslick , @benwandrew , @chadcwilliams and @gtdang – here's the updated model inventory PR for your perusal.

@hollandjg hollandjg self-assigned this Mar 6, 2023
Copy link
Collaborator

@gtdang gtdang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! I think this is so clear and easy to use. I just left one suggestion for improvement.

Copy link
Collaborator

@musslick musslick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I like the idea of including the docstrings. Other than that, happy to get it merged and then will start filling the data folder with more models :)

@chadcwilliams
Copy link
Collaborator

This is very well done! It is all very intuitive and I could follow along all of the files without a hitch. I had no issues, so you definitely have my thumbs up.

Copy link
Collaborator

@benwandrew benwandrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super sleek! let’s start growing the inventory.

@hollandjg hollandjg requested a review from gtdang March 9, 2023 19:19
Copy link
Collaborator

@gtdang gtdang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great John! I like how you support so many ways of accessing information with the dispatch. Very user friendly! Just had some questions for my understanding.


from autora.variable import VariableCollection


@runtime_checkable
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this decorator do?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That means that you can do an isinstance(x, _SyntheticExperimentClosure) call and check whether x fulfills the Protocol.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh cool! Thanks!

Comment on lines +181 to +205
@singledispatch
def describe(arg):
"""
Print the docstring for a synthetic experiment.

Args:
arg: the experiment's ID, an object returned from the `retrieve` function, or a closure
which creates a new experiment.
"""
raise NotImplementedError(f"{arg=} not yet supported")


@describe.register
def _(closure: _SyntheticExperimentClosure):
print(closure.__doc__)


@describe.register
def _(collection: SyntheticExperimentCollection):
describe(collection.closure)


@describe.register
def _(id_: str):
describe(retrieve(id_))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool! Didn't know about this! Is my understanding correct that if the object type is not one of the of the registered types it will default to the base (first) function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! That's it. It's just a different way of doing

if isinstance(x, SyntheticExperimentCollection): 
    ... do something ... 
elif isinstance(x, str):
    ... do something else ...
else:
    ... throw an error

which neatly leverages the type hints.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat!

@hollandjg hollandjg added this pull request to the merge queue Mar 10, 2023
Merged via the queue into main with commit 86c4d13 Mar 10, 2023
@hollandjg hollandjg deleted the feat/model-inventory branch March 10, 2023 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: integrate synthetic models into the project library
5 participants